New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop protobuf support for OpenMetrics #2098

Merged

mfpierre merged 2 commits into master from mfpierre/openmetrics-drop-protobuf

Aug 29, 2018

Contributor

mfpierre commented Aug 23, 2018 •

edited

Loading

Drop protobuf support for OpenMetrics (text only)

What does this PR do?

Drop open metrics protobuf support
Use native python client core.Metric objects instead of protobuf objects

ProtobufMetricFamily (old)	core.Metric (new)
name	name
type (int)	type (string)
help (string)	documentation (string)
metric ([]Metric proto objects)	samples ([]tuples<name, tags, value>)

Motivation

Optimizing the text only support for open metrics

Review checklist

PR has a meaningful title or PR has the no-changelog label attached
Feature or bugfix has tests
Git history is clean
If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

mfpierre added containers integration/datadog_checks_base changelog/Changed labels

mfpierre requested a review from a team as a code owner

August 23, 2018 08:39

mfpierre force-pushed the mfpierre/openmetrics-drop-protobuf branch 2 times, most recently from 4909086 to 2ea956f Compare

August 23, 2018 09:36

mfpierre requested a review from a team

August 23, 2018 09:44

rishabh requested changes

View reviewed changes

datadog_checks_base/datadog_checks/checks/openmetrics/mixins.py Outdated

-                      # message.type is the index in this array
-                      # see: https://github.com/prometheus/client_model/blob/master/ruby/lib/prometheus/client/model/metrics.pb.rb
-                      self.METRIC_TYPES = ['counter', 'gauge', 'summary', 'untyped', 'histogram']
+                      self.METRIC_TYPES = ['counter', 'gauge', 'summary', 'histogram']

rishabh Aug 23, 2018

I think we can just make this a class constant?

datadog_checks_base/datadog_checks/checks/openmetrics/mixins.py Outdated

+                          custom_hostname = self._get_hostname(hostname, sample, scraper_config)
+                          tags = self._metric_tags(metric_name, val, sample, scraper_config, hostname=custom_hostname)
+                          if sample[self.SAMPLE_NAME].endswith("_sum"):
+                              self.gauge("{}.{}.sum".format(scraper_config['namespace'], metric_name), val, tags=tags, hostname=custom_hostname)
                           else:

rishabh Aug 23, 2018

Are we still collecting {}.{}.quantile metrics?

Contributor Author

mfpierre Aug 23, 2018

Yes it should work, I put the quantile in the .count like the brackets and histograms, didn't pay attention it was separated for summary, I'll change this

datadog_checks_base/datadog_checks/checks/openmetrics/mixins.py Outdated

+                          if label_name not in scraper_config['exclude_labels']:
+                              tag_name = label_name
+                              if label_name in scraper_config['labels_mapper']:
+                                  tag_name = scraper_config['labels_mapper'][label_name]

rishabh Aug 23, 2018

Let's simplify this to tag_name = scraper_config['labels_mapper'].get(label_name, label_name) while we're here.

Contributor Author

mfpierre Aug 23, 2018

There's probably some other improvement like this lying around with labels being a dict, I focused on making it work first 😁

rishabh Aug 23, 2018

Yeah, I'm trying to find some more hehe 😄
We might as well improve this class while we can!

rishabh reviewed

View reviewed changes

datadog_checks_base/datadog_checks/checks/openmetrics/mixins.py Outdated

+                              self.gauge("{}.{}.sum".format(scraper_config['namespace'], metric_name), val, tags=tags, hostname=custom_hostname)
+                          elif sample[self.SAMPLE_NAME].endswith("_count"):
+                              self.gauge("{}.{}.count".format(scraper_config['namespace'], metric_name), val, tags=tags, hostname=custom_hostname)
+                          elif sample[self.SAMPLE_NAME].endswith("_bucket") and scraper_config['send_histograms_buckets']:

rishabh Aug 23, 2018

We can probably just merge these two conditionals into:
elif sample[self.SAMPLE_NAME].endswith("_count") or (sample[self.SAMPLE_NAME].endswith("_bucket") and scraper_config['send_histograms_buckets']):

mfpierre force-pushed the mfpierre/openmetrics-drop-protobuf branch 2 times, most recently from 8438763 to 1769441 Compare

August 23, 2018 14:39

rishabh requested changes

View reviewed changes

datadog_checks_base/datadog_checks/checks/openmetrics/mixins.py

-                  def process_metric(self, message, scraper_config, metric_transformers=None):
+                          for sample in metric.samples:
+                              for label_name in scraper_config['_watched_labels'].intersection(set(sample[self.SAMPLE_LABELS].keys())):

rishabh Aug 23, 2018

With how many lookups we make to sample[self.SAMPLE_LABELS], we should probably just set it to a variable haha

And I think we can do this in multiple functions. What do you think?

Contributor Author

mfpierre Aug 23, 2018

@rishabh while I agree it'll be better for readability, I'm not sure about the memory footprint of a new variable containing labels.
the lookup for sample[self.SAMPLE_LABELS] is O(1)

rishabh Aug 23, 2018

Yep, the footprint isn't a concern, it's just readability :)

datadog_checks_base/datadog_checks/checks/openmetrics/mixins.py Outdated

-                          for label in metric.label:
-                              if label.name == scraper_config['label_to_hostname']:
-                                  return label.value
+                          if scraper_config['label_to_hostname'] in sample[self.SAMPLE_LABELS]:

rishabh Aug 23, 2018

We can just merge these two conditions together.

mfpierre force-pushed the mfpierre/openmetrics-drop-protobuf branch 2 times, most recently from 8d0f587 to 12cb681 Compare

August 23, 2018 16:55

rishabh requested changes

View reviewed changes

datadog_checks_base/tests/test_openmetrics.py Outdated

-                  _met = _rate.metric.add()
-                  _met.gauge.value = 42
+                  _rate = GaugeMetricFamily('my_rate', 'Random rate')
+                  _rate.add_metric([], 42)
                   check = mocked_prometheus_check
                   mocked_prometheus_scraper_config['rate_metrics'] = ["my_rate"]

rishabh Aug 23, 2018 •

edited

Loading

We can get rid of this since rate_metrics isn't a thing anymore.

mfpierre force-pushed the mfpierre/openmetrics-drop-protobuf branch from 12cb681 to a11fdb8 Compare

August 24, 2018 08:28


          Drop protobuf support for OpenMetrics (text only) and optimize for te…

99f2a1d

…xt support

mfpierre force-pushed the mfpierre/openmetrics-drop-protobuf branch from a11fdb8 to 99f2a1d Compare

August 28, 2018 12:54

codecov-io commented Aug 28, 2018 •

edited

Loading

Codecov Report

Merging #2098 into master will decrease coverage by 17.06%.
The diff coverage is 91.52%.

rishabh previously approved these changes

View reviewed changes


          Fix histogram & summaries float tags

14faa49

mfpierre dismissed rishabh’s stale review via

14faa49

August 29, 2018 12:35

rishabh approved these changes

View reviewed changes

mfpierre merged commit c21e8bd into master

mfpierre deleted the mfpierre/openmetrics-drop-protobuf branch

August 29, 2018 15:00

shraykay pushed a commit to shraykay/integrations-core that referenced this pull request


          Drop protobuf support for OpenMetrics (DataDog#2098)

70e910e

* Drop protobuf support for OpenMetrics (text only) and optimize for text support

nmuesch pushed a commit that referenced this pull request


          Drop protobuf support for OpenMetrics (#2098)

ed66ac6

* Drop protobuf support for OpenMetrics (text only) and optimize for text support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

containers integration/datadog_checks_base